[WIP] final query scheduling fixes and testing #500
Conversation
briandennis
changed the title
briandennis
changed the title
| justifyContent: 'flex-start' | ||
| }} | ||
| > | ||
| <b>Scheduled queries will not run unless you are logged in.</b> Click <a href="/login">here</a>{' '} |
There was a problem hiding this comment.
I find this statement confusing. The backend saves the user's access token, so that the scheduled queries stored in ~/.plotly/connector/setting.yaml run as soon as the backend is launched.
There was a problem hiding this comment.
Perhaps I'm misunderstand the behaviour here... If the Plot.ly tab says I'm "not logged in" it's still possible for the scheduled queries to run?
There was a problem hiding this comment.
The scheduled queries are run by the backend. The backend stores the user's access token, so that it can run the scheduled queries in ~/.plotly/connector/setting.yaml even if the user isn't logged in (e.g. after restating Falcon).
There was a problem hiding this comment.
OK awesome. So what does logging in do then? If the backend has my token, and I'm "logged out" why must I log in to create a scheduled query?
There was a problem hiding this comment.
This is my understanding of how authentication is used in Falcon v2.8 (i.e. no schedule panel):
-
if no
Let's encryptcredential has been generated and the user isn't logged in (i.e. the frontend hasn't gone through the oauth2 procedure), then the backend doesn't launch the HTTPS server. The user is required to log in to generate aLet's encryptcredential. The backend stores the generated credential, so that users won't have to log in and generate the certificate every time they start Falcon. -
if
AUTH_ENABLEDistrue(default) and Falcon is in web app mode, then users are required to log in before they can do anything and their username has to be one of those listed inALLOWED_USERS.
There was a problem hiding this comment.
Ah, I (more or less) understand, thanks!
There was a problem hiding this comment.
To be clear, this warning should be removed then, correct? 🙂
| <p>To create a scheduled query, you'll need to be logged into Plotly.</p> | ||
| <p> | ||
| To create a scheduled query, you'll need to be logged into Plotly. Logging in will reset your query, | ||
| so please save it elsewhere before doing so. |
There was a problem hiding this comment.
This is probably about as good as we can do I suppose. There's no way to squirrel the query away into local storage or a cookie or something and restore it after login?
There was a problem hiding this comment.
Happy to look into this on the next stage, but given time constraints, I don't feel comfortable implementing/testing this functionality before the release.
There was a problem hiding this comment.
OK. Any way we could make this warning very clear? put it on its own line, in bold? this is a pretty bad failure case if you've just iterated for an hour on some complex query.
| @@ -510,7 +510,7 @@ class Settings extends Component { | |||
| <p style={{textAlign: 'right'}}> | |||
| Not logged in | |||
| <br/> | |||
| <a onClick={() => window.location.assign('/login')}>Log into Plotly</a> | |||
| <a href="/login">Log into Plotly</a> | |||
There was a problem hiding this comment.
If you want to make this change, you also need to change the <a> above like this:
user@host:~/github/plotly-database-connector$ git diff
diff --git a/app/components/Settings/Settings.react.js b/app/components/Settings/Settings.react.js
index d46f1f8..ab0c608 100644
--- a/app/components/Settings/Settings.react.js
+++ b/app/components/Settings/Settings.react.js
@@ -504,7 +504,7 @@ class Settings extends Component {
<p style={{textAlign: 'right'}}>
{`Logged in as "${username}"`}
<br/>
- <a onClick={logout}>Log Out</a>
+ <a href="/" onClick={logout}>Log Out</a>
</p>
) : (
<p style={{textAlign: 'right'}}>Otherwise users will be redirected to the login page immediately after logging out:
There was a problem hiding this comment.
I am just reverting this. Not a necessary change. Thanks 👍
| const ONE_DAY = 86400; | ||
| const ONE_WEEK = ONE_DAY * 7; | ||
|
|
||
| if (currentDay !== 3) { |
There was a problem hiding this comment.
I don't understand the purpose of this if. It looks like it was meant to be a while to move currentDay back to Wednesday.
| const ONE_WEEK = ONE_DAY * 7; | ||
|
|
||
| if (currentDay !== 3) { | ||
| // timezone is UTC or later, go back |
There was a problem hiding this comment.
I don't understand why the comment about the timezone is relevant.
| if (currentDay !== 3) { | ||
| // timezone is UTC or later, go back | ||
| // to Wednesday | ||
| clock.tick(-1 * ONE_DAY * 1000); |
There was a problem hiding this comment.
Is this possible? yes, it is
While looking at sinon's API for fake timers, I've noticed the fake clock is initialiased at UNIX epoch.
This is relevant for the tests below, because it means currentMinute is 0.
| connectionId: '...' | ||
| }); | ||
|
|
||
| if (currentMonth !== 0) { |
There was a problem hiding this comment.
while? anyway, not necessary since clock is initialiased at UNIX epoch.
test/backend/QueryScheduler.spec.js
Outdated
| const spy = sinon.spy(); | ||
| queryScheduler.job = spy; | ||
| queryScheduler.scheduleQuery({ | ||
| // every day at 15 minutes past the current hour on |
|
@n-riesco re timezones: Yes, the fake timers are always initialized to the UNIX epoch (12:00 AM UTC on Thursday, 1 January 1970). However, timezones do still have an effect. For example, I'm currently on Pacific Time which is 8 hours behind UTC. This means when I run the tests on my machine, the fake date objects are set to 4:00 PM on Wednesday, 31 December 1969. The |
|
After going down a bit of a rabbit hole, I think the CI test error is due to an independent issue with the bumped version of It looks like that package is failing to import a file called LineSelectors ( |
|
@briandennis we can workaround this casing issue by defining an alias in webpack's config. This works for me: diff --git a/webpack.config.base.js b/webpack.config.base.js
index 5e19e50..bb7c51e 100644
--- a/webpack.config.base.js
+++ b/webpack.config.base.js
@@ -24,6 +24,13 @@ export default {
libraryTarget: 'commonjs2'
},
resolve: {
+ alias: {
+ // workaround for react-chart-editor@0.21
+ './LineSelectors$': path.resolve(
+ __dirname,
+ 'node_modules/react-chart-editor/lib/components/fields/lineSelectors.js'
+ )
+ },
extensions: ['.js', '.jsx']
},
plugins: [EDIT: No need for this; instead, wait for next patch release of react-chart-editor. |
|
@jakedex Thank you for cleaning up our scrollbars (it was long overdue!) |
package.json
Outdated
| @@ -247,6 +247,7 @@ | |||
| "papaparse": "^4.3.7", | |||
| "pg": "^4.5.5", | |||
| "pg-hstore": "^2.3.2", | |||
| "react-copy-to-clipboard": "^5.0.1", | |||
| Log In | ||
| </button> | ||
| <CopyToClipboard text={props.preview.code}> |
There was a problem hiding this comment.
Nice!
When I tested it, I noticed the button doesn't give any visual feedback that the copy to clipboard succeeded.
There was a problem hiding this comment.
+1. would be nice to get an indication that copying has suceeded
| @@ -104,6 +104,9 @@ class CreateModal extends Component { | |||
| if (!this.state.interval) { | |||
| return this.setState({error: 'Please select an interval above.'}); | |||
| } | |||
| if (this.state.name && this.state.name.trim().length === 0) { | |||
There was a problem hiding this comment.
I haven't debugged the reason, but this isn't working for me. I'm still able to create queries without a name:
There was a problem hiding this comment.
That’s by design I think...
There was a problem hiding this comment.
Er... perhaps not, looking at the code. I should have said “I had assumed it was by design that you can create queries without names and it didn’t bother me”
There was a problem hiding this comment.
Well, it looks like you can create queries named "" but not " "... The first part of the if condition is falsey for "". Given that we still have to support unnamed queries, we may as well just trim and move on if the user tries " " instead of complaining :)
There was a problem hiding this comment.
@briandennis How about if ((this.state.name || '').trim().length === 0)?
There was a problem hiding this comment.
My preference here would be to allow any trimmed string, even empty ones, but I'm also OK with the current behaviour if we're out of time.
| </Row> | ||
| </Column> | ||
| </Modal> | ||
| ); | ||
|
|
||
| PromptLoginModal.propTypes = { | ||
| open: PropTypes.bool.isRequired, | ||
| preview: PropTypes.string.isRequired, |
There was a problem hiding this comment.
This doesn't look right. <CopyToClipboard> uses props.preview.code.
| @@ -115,6 +115,7 @@ class Scheduler extends Component { | |||
| initialCode: PropTypes.string, | |||
| requestor: PropTypes.string, | |||
| dialect: PropTypes.string, | |||
| preview: PropTypes.string, | |||
There was a problem hiding this comment.
preview: PropTypes.object,
|
Oh man I’m so sorry about the LineSelector thing... I had seen that while reviewing the change but it worked with web pack locally so I didn’t investigate further. Sorry that something that should have been trivial took so long. I’ll push out a point version with a fix when I get to the office. |
|
Huh, on second look, there's nothing wrong in the The problem was that my process for building a new |
|
OK, version |
|
Tested react-chart-editor@0.21.1 and works for me:
|
|
Cool! BTW when this hits master it'll resolve #479 ... in your gif above you'll notice that trace-type is no longer offered. |
|
I'm loving the copy-to-clipboard thing, thanks for implementing that on short notice. It's not as good as not nuking the query but it's a nice touch :) |
|
@nicolaskruchten no worries on the |
#499 as well as other final fixes
Changes